Skip to content

Conversation

@sratz
Copy link
Member

@sratz sratz commented Nov 3, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

Test Results

 1 926 files   -  21   1 926 suites   - 21   3h 4m 22s ⏱️ + 1h 18m 42s
 4 730 tests +  9   4 694 ✅  -   3   24 💤 ±0  3 ❌ +3   9 🔥 + 9 
14 037 runs   - 126  13 835 ✅  - 161  168 💤 +1  7 ❌ +7  27 🔥 +27 

For more details on these failures and errors, see this check.

Results for commit c72d4b3. ± Comparison against base commit 879c3bd.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Contributor

laeubi commented Nov 4, 2025

@sratz you need suppress the illegal access here, the testfailure look suspicious...

@sratz
Copy link
Member Author

sratz commented Nov 4, 2025

@sratz you need suppress the illegal access here, the testfailure look suspicious...

The problem is

@RegisterExtension
SessionTestExtension extension = SessionTestExtension.forPlugin(PI_RUNTIME_TESTS)
.withCustomization(SessionTestExtension.createCustomConfiguration().setCascaded().setReadOnly()).create();

which uses SessionTestExtension with CustomSessionConfigurationImpl

I have added org.Mockito as a dependency, which now cannot be resolved with the default of set bundles configured by CustomSessionConfigurationImpl.

Where should I add those additional bundles? customize just PlatformURLSessionTest or adjust the default in CustomSessionConfigurationImpl?

@laeubi
Copy link
Contributor

laeubi commented Nov 4, 2025

Mockito might be used in more tests in the future so I think you can decide what works best for you here.

@akurtakov
Copy link
Member

I have just read eclipse-jdt/eclipse.jdt.core#4587 which somehow made me more cautious about mocking frameworks.

@HeikoKlare
Copy link
Contributor

Where should I add those additional bundles? customize just PlatformURLSessionTest or adjust the default in CustomSessionConfigurationImpl?

If we can assume that every consumer of the session test framework has Mockito available, it would probably make sense to add it to the default bundles. Note that they are used in other projects as well (such as Equinox), but I guess all of them will have Mockito available.

@sratz
Copy link
Member Author

sratz commented Nov 4, 2025

I have just read eclipse-jdt/eclipse.jdt.core#4587 which somehow made me more cautious about mocking frameworks.

True, mocking concrete classes (especially final ones) / static mocks / etc. can be dangerous and are often result of a bad design. Rules of good design are typically:

  • Limit mocking to interfaces
  • Limit mocking to first-order / direct dependencies of the class-under-test.

In this case, X509Certificate is abstract with a bunch of abstract methods. Writing CollectionTrustManagerTest without mocks would be quite the overhead - especially since the tests does not care about the implementation at all.

@sratz
Copy link
Member Author

sratz commented Nov 4, 2025

I added the needed bundles to just the one test class needing them.

Alternatively, use custom stubs instead of mocks and the need for Mockito:

With vs. without mocks:
e4ca190...c72d4b3

I have no real preference, but the latter would avoid the dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants